Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: update Algolia dependencies #2689

Merged
merged 8 commits into from
Feb 6, 2018
Merged

Conversation

bobylito
Copy link
Contributor

@bobylito bobylito commented Feb 2, 2018

Adds support for insights in the helper.

@Haroenv
Copy link
Contributor

Haroenv commented Feb 2, 2018

Does renovate not work here?

@algobot
Copy link
Contributor

algobot commented Feb 2, 2018

Deploy preview for algolia-instantsearch ready!

Built with commit 7b4a053

https://deploy-preview-2689--algolia-instantsearch.netlify.com

@vvo
Copy link
Contributor

vvo commented Feb 2, 2018

package.json must be updated along with the lock file too for this to work well. The yarn.lock file is not read when you install/check the dependencies as a user.

i.e. it means that if you release InstantSearch.js with this package.json and I update InstantSearch.js in my project, if my local node_modules says helper 2.23.0 then it won't be updated (but the feature you rely on won't work too)

@vvo
Copy link
Contributor

vvo commented Feb 2, 2018

And yes might be good to know why the helper has not been updated in package.json for so long, maybe @rarkins know why. The branches are protected so there should be PRs being opened (on weekends) but that's maybe no more the case now

2.3.0 was released long time ago so should have been updated already

Update: it's expected because it will be updated only this weekend.

@rarkins
Copy link

rarkins commented Feb 2, 2018

@vvo are you referring to algoliasearch-helper? Its current version in dependencies is ^2.23.0, and the latest on npmjs is v2.24.0, hence it is "in range" and not eligible for an upgrade.

If you want it bumped to the exact latest version every time, there's really no benefit to any end user in having the caret ^ in place in package.json, so you can remove it and see it updating regularly.

By this I mean to say: Why use ranges at all? Usually it's in case a library is "consumed" downstream and you want to give users as wide a choice of dependencies as possible, so that's why we assume you don't want it updated to the latest. Otherwise having a range but keeping it updated to the very latest release every time is effectively the same as having no range at all.

Another alternative for you is to enable lock file maintenance, this bumps lock files even if the package.json remains the same. i.e. would achieve something similar to this PR, for every single package.

@vvo
Copy link
Contributor

vvo commented Feb 2, 2018

@rarkins I believe if we wait this weekend, the helper will be bumped to ^2.24.0 by renovate right?

@rarkins
Copy link

rarkins commented Feb 2, 2018

@rarkins I believe if we wait this weekend, the helper will be bumped to ^2.24.0 by renovate right?

No, that's not the behaviour actually. The logic is: "if the latest npmjs release falls within the existing range, do not update it". This is because in most cases, people use ranges because they want to keep a wide range for downstream users.

If you need it bumped to >= v2.24.0 for functional reasons (e.g. using a feature of v2.24.0 that's not supported by v2.23.0 then that's something a developer needs to do manually, because only the developer involved could know that. If instead you just think it's best practice to keep your downstream users as updated as possible (e.g. to the latest, once a week), is there any reason why not to remove the caret range and pin it?

Another option is to use tilde ~ ranges instead, then it would bump from ~2.3.0 to ~2.4.0 but not to ~2.3.1 for example.

And finally.. this "logic" I mentioned is not written in stone and I can consider a feature request if you wish to keep bumping the range ever week. I just prefer we both understand "why" things are like they are.. my philosophy is that whenever you set out solving the wrong problem, you're always going to end up with a wrong solution :)

In short: you use ranges if you don't want to overly restrict your downstream users about which (compatible) versions they should use. If you do wish to be restrictive, then a narrower range and/or pinning sounds better.

Let me know if the above is clear or any parts are unclear or disagreeable?

@samouss
Copy link
Contributor

samouss commented Feb 2, 2018

When we rely on such tools (like Renovate) we should go for exact dependencies.

When we don't want to upgrade just close the PR. Correct me if I'm wrong but when you close a PR Renovate will ignore the dependency until a new version is released.

@rarkins
Copy link

rarkins commented Feb 2, 2018

@samouss yes that's right. e.g. if existing is v2.3.0 and Renovate suggests to upgrade to v2.4.0 but you close it, then Renovate will ignore v2.4.0 forever. If v2.4.1 or v2.5.0 comes out then it will then suggest it again and you have the choice to merge or ignore.

@bobylito
Copy link
Contributor Author

bobylito commented Feb 3, 2018

Thanks @haroen, @samouss and @vvo for picking up this and asking questions 🤘

The reasoning here is that I was unsure about the behavior of renovate (thanks @rarkins for the explanation here 😅) and I wanted to be sure that this dependency is updated correctly for Tuesday release. It contains the new insight feature.

Clearly @vvo there is an issue with the package.json 😅 I will fix that according to renovate behavior.

#teamwork

@bobylito bobylito requested review from vvo and samouss February 5, 2018 14:35
Copy link
Contributor

@samouss samouss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 We could also add a .yarnrc with:

# .yarnrc
save-prefix false

Will pin the dependencies by default when we add a new one.

package.json Outdated
"prop-types": "^15.5.10",
"to-factory": "^1.0.0"
"classnames": "2.2.5",
"events": "1.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that as long as we don't do a release, people will be stuck on the older version of these, while with the ^, they'd still get upgraded, even if we don't update yet. I wonder if that's a problem 🤔

@vvo
Copy link
Contributor

vvo commented Feb 5, 2018

Thanks for all the details, now I am in favour of having dependencies pinned in all libraries.

@bobylito bobylito merged commit 96b8d61 into develop Feb 6, 2018
@bobylito bobylito deleted the chore/update-helper-2.4 branch February 6, 2018 12:21
bobylito pushed a commit that referenced this pull request Feb 6, 2018
<a name=2.5.0></a>
# [2.5.0](v2.4.1...v2.5.0) (2018-02-06)

### Bug Fixes

* **doc:** add maximum width to images (fix [#2685](#2685)) ([#2686](#2686)) ([f4b5377](f4b5377))

### Features

* support for algolia insights ([#2689](#2689)) ([96b8d61](96b8d61))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants